-
Notifications
You must be signed in to change notification settings - Fork 166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace equalArrays
with lodash.isEqual
#1691
Conversation
The custom `equalArrays` function can raise an error when one of the provided arguments does not have a `length` property. Instead of updating the function, just replace with `lodash.isEqual` which DTRT. Fixes #1690
This reverts commit 3b16854.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing to lodash's isEqual
does fix this, however it also allows non-array arguments which is not the intention of the code. The underlying bug is that changeEntropyCdsSelection
should be a no-op if the entropy data isn't present, i.e.
--- a/src/actions/entropy.js
+++ b/src/actions/entropy.js
@@ -55,6 +55,7 @@ export const updateEntropyVisibility = debounce((dispatch, getState) => {
export const changeEntropyCdsSelection = (arg) => (dispatch, getState) => {
const action = {type: types.CHANGE_ENTROPY_CDS_SELECTION}
const entropy = getState().entropy;
+ if (!entropy.loaded) return;
This is a good example of where TypeScript would have helped ensure arguments passed to equalArrays
were indeed arrays, which was the intention of the code. We don't want cases where we pass two non-array-but-equal arguments to isEqual
.
While the intention of the code was to never call equalArrays with non-array arguments, we should also improve equalArrays
to catch cases where we didn't do the right thing:
export const equalArrays = (a,b) => Array.isArray(a) && Array.isArray(b) && a.length===b.length && a.every((el, idx) => b[idx]===el);
I'll do a little more digging to see if there are other situations where we assume the presence of entropy data.
The function should exit immediately if no entropy data is loaded from the dataset. This is the underlying bug that caused code to pass non-arrays to the final equality check. Co-authored-by: James Hadfield <hadfield.james@gmail.com>
Hmm, this means it will return False if either arg is not an array and when the arrays are not equal which can make falsy checks problematic auspice/src/components/entropy/index.js Lines 231 to 232 in cdf9f7c
I think it would be safer to do type checks separately than equality checks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this means it will return False if either arg is not an array and when the arrays are not equal which can make falsy checks problematic
Yes, that's fair, although isEqual
will do the same. I think typescript is the way to go here, but that's well out of scope for this fix.
Description of proposed changes
The custom
equalArrays
function can raise an error when one of the provided arguments does not have alength
property. Instead of updating the function, just replace withlodash.isEqual
which DTRT.Related issue(s)
Fixes #1690
Testing